-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: show tlv records in transaction popup #365
Conversation
I am not sure how the data processing works there. can we get that message on the transaction list? where we have the "description" ? cc @rolznz |
Yes we can, but we have to discuss on the priority order |
Sounds good (and the description would be empty in this case I think anyway) |
but we would need to persist that data in the DB. we should not on the fly always decode those keysends. that's why we have the DB. prepare the data as our app needs it. |
@bumi @im-adithya I think we should go with Bumi's recommendation, right? @im-adithya would you like to propose the changes you would make? or how should we proceed here? |
TODOs
|
transactions/transactions_service.go
Outdated
@@ -607,14 +620,19 @@ func (svc *transactionsService) ConsumeEvent(ctx context.Context, event *events. | |||
|
|||
if result.RowsAffected == 0 { | |||
// Note: brand new payments cannot be associated with an app | |||
var metadata string | |||
var metadataBytes []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think we should do this on nwc_payment_sent
because we already save the metadata when we initiate the payment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that important but what if payments happen outside (they don't have metadata since it is a payment, atleast in LND but still I thought it was good to have if we are anyways adding metadata)
Also we should DRY up the "brand new" lnClient.Transaction processing because it is almost the same in both sent/received
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to remove it for now - we need to confirm if we want to add this support and if so, also import transactions from LND (all this is a lot of extra work and code to do properly)
- only migrate values that are non-empty - wrap in db transaction
Example queries:
|
Roland's tests (WIP):
|
Name string `json:"name"` | ||
Podcast string `json:"podcast"` | ||
URL string `json:"url"` | ||
Episode string `json:"episode,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@im-adithya why do some have omitempty others not? is it required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this from LBE repo here, whereever we have ?:
in the Boostagram type there, I've omitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it looks completely arbitrary and doesn't seem to follow the spec - https://github.com/lightning/blips/blob/master/blip-0010.md#fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
Fixes #31
Fixes #372
Adds custom record info in the transaction popup
Screenshot